fix(fb_custom_audience): one invalid event causes entire batch to fail#5036
fix(fb_custom_audience): one invalid event causes entire batch to fail#5036shekhar-rudder merged 2 commits intodevelopfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Record transform (FB Custom Audience) src/v0/destinations/fb_custom_audience/recordTransform.ts |
Reworked error handling: processRecord now returns `{ metadata } & ({ dataElement[] } |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and specifically describes the main change: fixing an issue where invalid events in a batch caused the entire batch to fail, which is the primary objective of this PR. |
| Description check | ✅ Passed | The PR description provides a clear summary of changes, test coverage details, and confirms all tests pass. However, it lacks several template sections including Linear task reference, technical considerations, and developer checklist completion. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
int-5922-fb-audience-partial-error
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/v0/destinations/fb_custom_audience/recordTransform.ts (1)
119-145: Consider whether async iteration is necessary for synchronous processing.The inner
forEachInBatchesat line 121 wraps a synchronousprocessRecordcall. This introduces unnecessary async overhead. A regularforEachorfor...ofloop would be more efficient here.♻️ Suggested refactor
await forEachInBatches(recordChunksArray, async (recordArray) => { const data: unknown[][] = []; - await forEachInBatches(recordArray, async (input) => { - const result = processRecord( + for (const input of recordArray) { + const result = processRecord( input, userSchema, isHashRequired, disableFormat, input.metadata.workspaceId, destination.ID, ); if (result.error) { const error = new InstrumentationError(result.error); const errorObj = generateErrorObject(error); invalidEvents.push( getErrorRespEvents( [result.metadata], errorObj.status, errorObj.message, errorObj.statTags, ), ); } else { data.push(result.dataElement!); metadata.push(result.metadata); } - }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/recordTransform.ts` around lines 119 - 145, The inner async batch iterator is unnecessary because processRecord is synchronous; replace the inner await forEachInBatches(recordArray, async (input) => { ... }) with a synchronous loop (e.g., for...of or Array.prototype.forEach) over recordArray, call processRecord directly, and push to data/metadata or invalidEvents as before, removing the async callback and the extra await to avoid unnecessary async overhead; keep outer forEachInBatches over recordChunksArray intact and preserve usage of variables recordArray, processRecord, invalidEvents, data, and metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/v0/destinations/fb_custom_audience/recordTransform.ts`:
- Around line 119-145: The inner async batch iterator is unnecessary because
processRecord is synchronous; replace the inner await
forEachInBatches(recordArray, async (input) => { ... }) with a synchronous loop
(e.g., for...of or Array.prototype.forEach) over recordArray, call processRecord
directly, and push to data/metadata or invalidEvents as before, removing the
async callback and the extra await to avoid unnecessary async overhead; keep
outer forEachInBatches over recordChunksArray intact and preserve usage of
variables recordArray, processRecord, invalidEvents, data, and metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b043191-611c-40f8-a568-ae4975261ee2
⛔ Files ignored due to path filters (2)
test/integrations/destinations/fb_custom_audience/router/data.tsis excluded by!**/test/**test/integrations/destinations/fb_custom_audience/router/rETL.tsis excluded by!**/test/**
📒 Files selected for processing (1)
src/v0/destinations/fb_custom_audience/recordTransform.ts
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5036 +/- ##
========================================
Coverage 92.27% 92.27%
========================================
Files 657 657
Lines 35941 35956 +15
Branches 8480 8485 +5
========================================
+ Hits 33163 33178 +15
Misses 2540 2540
Partials 238 238 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/v0/destinations/fb_custom_audience/recordTransform.ts`:
- Around line 121-144: Wrap the call to processRecord inside a try/catch so
thrown exceptions per record are caught and converted to invalidEvents instead
of rejecting the whole batch: call processRecord(...) inside try, handle the
existing 'error'-in-result branch as-is, and in catch create an
InstrumentationError from the thrown error, call generateErrorObject on it, then
push getErrorRespEvents using input.metadata (since result may not exist) into
invalidEvents; ensure on catch you continue the loop (do not rethrow) so other
records in the batch are processed. Use the existing symbols: forEachInBatches,
processRecord, InstrumentationError, generateErrorObject, getErrorRespEvents,
invalidEvents, data, and metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 741fc232-1c49-4a44-82a7-54b69e6c0956
📒 Files selected for processing (1)
src/v0/destinations/fb_custom_audience/recordTransform.ts



Summary
processRecordnow returns an error field instead of throwing, andprocessRecordEventArrayseparates valid/invalid records into success and error responsesTest plan
npm run test:ts -- component --destination=fb_custom_audience)npx jest --testPathPattern="fb_custom_audience")🤖 Generated with Claude Code